I know I'm the one who suggested this initially but I think we should avoid Kotlin APIs (like sealed classes) in the Gradle plugin API.

Mainly for Groovy users + because we use R8 to relocate all of the Kotlin-stdlib, this feels dangerous. I'd suggest something like this instead:

class Service {
  fun mapScalar(graphQLName: String, kotlinName: String)
  fun mapScalarUsingKotlinSingleton(graphQLName: String, kotlinName: String, singletonName: String)
  fun mapScalarUsingKotlinConstructor(graphQLName: String, kotlinName: String, constructorName: String)

  fun mapScalarToKotlinLong(graphQLName: String)
  fun mapScalarToKotlinInt(graphQLName: String)
  fun mapScalarToKotlinString(graphQLName: String)
  fun mapScalarToKotlinBoolean(graphQLName: String)
  fun mapScalarToKotlinMap(graphQLName: String)

  fun mapScalarToJavaLong(graphQLName: String)
  fun mapScalarToJavaInteger(graphQLName: String)
  fun mapScalarToJavaString(graphQLName: String)
  fun mapScalarToJavaBoolean(graphQLName: String)
  fun mapScalarToJavaMap(graphQLName: String)
}

Agreed!
Plus I actually find the syntax easier to use 👍

Shouldn't mapScalarUsingKotlinSingleton / mapScalarUsingKotlinConstructor be mapScalarUsingSingleton / mapScalarUsingConstructor?

Shouldn't mapScalarUsingKotlinSingleton / mapScalarUsingKotlinConstructor be mapScalarUsingSingleton / mapScalarUsingConstructor?

I was wondering the same. Singleton is a Kotlin concept so it will not work with Java but maybe we can throw a nice error if that happens?

I was assuming this would work also in Java with static fields:

public class MyAdapters {
  public static final Adapter<MyDate> DATE_ADAPTER = new Adapter<MyDate>() {...}
}

mapScalarUsingSingleton("Date", "com.example.MyDate", "com.example.MyAdapters.DATE_ADAPTER")

But now I'm realizing maybe the name "singleton" is not the most accurate?

Maybe we can "just" ask for a verbatim code block?

apollo {
  mapScalarUsingAdapter("Date", "com.example.MyDate", "com.example.MyAdapters.DATE_ADAPTER")
  mapScalarUsingAdapter("Date", "com.example.MyDate", "com.example.MyAdapterSingleton")
  mapScalarUsingAdapter("Date", "com.example.MyDate", "com.example.MyAdapter()")
}

And use a literal (%L) codeblock in KotlinPoet/JavaPoet. This will not add the nice imports but maybe that's ok?

I think I agree, the distinction is not that useful. The documentation can explain how it works, and the fact that an instance or an instantiation expression can both be used.

In that case, maybe we can keep the name mapScalar?

  /**
   * Map a GraphQL scalar type to the Java/Kotlin type and adapter configured at runtime.
   * 
   * For example:
   * `mapScalar("Date", "com.example.Date")`
   */
  fun mapScalar(graphQLName: String, targetName: String)

  /**
   * Map a GraphQL scalar type to the Java/Kotlin type and provided adapter expression.
   *
   * For example:
   * `mapScalar("Date", "com.example.Date", "com.example.DateAdapter")` (an instance property or object)
   * `mapScalar("Date", "com.example.Date", "com.example.DateAdapter()")` (instantiate the class on the fly)
   */
  fun mapScalar(graphQLName: String, targetName: String, adatpterExpression: String)

mapScalarToKotlinMap / mapScalarToJavaMap should be mapScalarToKotlinAny / mapScalarToJavaObject or am I missing something?

Yes, indeed. Until we have #3243, we can't use Map or other parametrized classes.

This feels dangerous. What if there's a conflicting mapping in 2 modules? Should we mandate that all scalar mappings are defined in the schema module?

I can't think of any use case where different modules could use different mappings but if something comes up we can always relax the restriction.

I thought it was already the case actually. The multi module doc says "The schema module and only the schema module must define customScalarsMapping".

Should I ensure there are no duplicates and throw if there are any? Or should we not merge them at all (not sure how that would work)?

Better throw fast if we see something like this.

Why does it need to be Serializable ?

Got it. This is need by Gradle to snapshot the input properties.

I always feel weird implementing Serializable since it's already serialized to Json. Also using Kotlin features like sealed interfaces and data classes in Gradle APIs feels dangerous too (think Groovy callers).

I would certainly have used 2 maps:

scalarsMapping: Map<String, String>
scalarAdapters: Map<String, String>

But I can't find a strong reason for the change so your choice!

Yes sorry I should have commented :). I agree, I'm not very fond of making them as Serializable either. Using String feels safer, I'll make the change.

Thinking a bit more about this. I think it makes sense to keep the builtin types in apollo-api always and not generate them. This means for builtin types that require a runtime adapter, users need to do something like:

apolloClientBuilder.addScalarAdapter(CompiledIDType, MyIDAdapter)
// instead of
// apolloClientBuilder.addScalarAdapter(ID.type, MyIDAdapter)

But I think this is such an advanced case that it's ok? It keeps the logic more symmetric. Builting types are in apollo-api, others get generated

👍 I'll have a look!

In retrospect, CompiledIDType should certainly have been BuiltinIDType but that'll be for v4 😅

Hmm after more digging, the CustomScalarAdapters methods take instances of CustomScalarTypes, whereas the compiled types are ScalarType - so apolloClientBuilder.addScalarAdapter(ID.type, MyIDAdapter) for instance doesn't work.

We could make CustomScalarType be a subclass of ScalarType, and change CustomScalarAdapters methods to take ScalarType instead. But that looks like a breaking change 😅

Or we can add ScalarType based methods (responseAdapterFor, add, addCustomScalarAdapter) in addition to the existing CustomScalarType based ones.

WDYT?

Gosh this is hard. I'm not too much a fan of duplicating everything either. I think your initial approach of generating a CustomScalarType for builtin types if they have a mapping was best then. CustomScalarType will not be a super accurate name and the whole thing feels a bit asymmetrical but I don't see a better way.

Or should we always generate all builtin types? And mark CompiledStringType, etc.. as deprecated? I was initially not too fond of this because it puts String, Int, etc.. as top level classes but maybe since they're using a specific package name, that'll be ok...

Or should we always generate all builtin types?

I'll five it a go so we can see what it looks like :)

throw if both customScalarsMapping and scalarMapping are set?

Good point!

Rename CustomScalarTarget to ScalarTarget?

I'd love this to be moved up a few layers so that the resolver can apply the same logic everywhere. Not really sure how to do this yet but I might investigate this in a follow up PR unless you have ideas how to do this already.